ci: nightly E2E workflow for the Thrift and kernel backends#402
Conversation
ea1378e to
558f46c
Compare
4929e40 to
2a43d24
Compare
558f46c to
ba7b5d9
Compare
2a43d24 to
ebedfc7
Compare
ba7b5d9 to
2922c72
Compare
ebedfc7 to
23de162
Compare
2922c72 to
4b7b202
Compare
23de162 to
fb098a1
Compare
|
One operational concern: the kernel E2E job has a 40-minute job timeout, but it first runs a potentially cold On a cache miss or slow kernel build, the build only needs to take more than ~10 minutes before GitHub can kill the job before Go's timeout produces useful diagnostics about which E2E test hung or failed. Could we either raise the job timeout to something like 60-70 minutes, or split the budget explicitly, e.g. a timeout around |
|
Fixed in Went with the explicit-split option you suggested:
Worst case is now 25 (build) + 30 (go test) + setup < 65, so a wedged build is killed distinctly as a build-step failure, and a hung test still hits Go's own This comment was generated with GitHub MCP. |
6376422 to
4512646
Compare
ef7e5ec to
752e19b
Compare
4512646 to
091accc
Compare
752e19b to
125eec7
Compare
091accc to
ac112d4
Compare
Add a scheduled (07:00 UTC) + manually dispatchable workflow that runs the
credential-gated end-to-end suites against a real test SQL warehouse. The
ordinary "Go" workflow injects no warehouse secrets, so every E2E test t.Skip()s
there and real-warehouse behaviour — large multi-page CloudFetch, S3 downloads,
drain-past-deadline, real auth — is otherwise not exercised in CI.
Two jobs against one test warehouse:
- thrift-e2e: pure-Go (CGO_ENABLED=0), the high-value CloudFetch drain +
exact-row-count scenarios.
- kernel-e2e: reuses the go.yml kernel-build machinery (JFrog/cargo proxy,
pinned Rust, kernel App-token clone, kernel-lib/cargo caches), then runs the
databricks_kernel-tagged E2E funcs plus the Thrift-vs-kernel parity funcs
(which need both the kernel lib and live credentials).
Both suites now read the same DATABRICKS_PECOTESTING_* credentials, so one secret
set drives the whole workflow — the kernel E2E / parity test helpers are updated
from the ad-hoc DATABRICKS_HOST/_HTTP_PATH/_TOKEN vars to the PECOTESTING set
(with the _TOKEN_PERSONAL fallback the Thrift suite already uses). Each job has a
fail-loud credential guard so a missing secret errors instead of silently
skipping into a misleading green. TestKernelE2EM2M is excluded (-skip) since it
needs a service-principal client id/secret the warehouse secret set doesn't
carry; the job comment documents how to enable it.
Alerting is GitHub's built-in failed-scheduled-run notification. Provisioning the
DATABRICKS_PECOTESTING_* secrets in the repository is the remaining step to make
the nightly run green.
Closes PECOBLR-3308.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…fires first The kernel E2E job had a 40-min job cap while `go test` alone was given -timeout 30m, leaving only ~10 min for checkout, toolchains, caches, and a potentially cold ~200-crate `make kernel-lib` Rust build. On a cache miss the build can exceed that, so GitHub SIGKILLs the whole job at 40 min before Go's timeout can emit its which-test-hung goroutine dump — losing the diagnostics. Split the budget explicitly: raise the job cap to 65 min and add a 25-min per-step timeout on `make kernel-lib`. Worst case is 25 (build) + 30 (go test) + setup < 65, so a wedged build is killed distinctly as a build failure and a hung test still hits Go's own -timeout first. Also moved the E2E step's descriptive comment down onto the Run step it documents. Co-authored-by: Isaac
125eec7 to
aa27368
Compare
ac112d4 to
0c68dfb
Compare
0c68dfb
into
mani/sea-kernel-consolidated
…tly workflows for e2e tests (#399) ## What Extends the SEA-via-kernel backend for the Go driver with the full DAIS gap-closure + PuPr feature set: authentication, session setup, type rendering, query execution, telemetry, richer TLS, kernel-log routing, and CI. This PR consolidates what was a 5-PR stack (#400 richer-TLS, #401 log-unify, #402 nightly-e2e, #403 c2-dispatch) into a single reviewable PR. The stack was linear, so the branches were fast-forwarded into this one — every change is preserved as a **distinct, logically-scoped commit** (23 commits), so it reads commit-by-commit rather than as one blob. #400–#403 now show as merged (their commits live here). ## Features **Authentication** - **OAuth M2M / U2M** — the kernel drives its own auth flow from `cfg.Authenticator` (mirroring pyo3/napi). Adds a `kernel.Auth` descriptor + `resolveKernelAuth`, wired through the `set_auth_pat` / `_m2m` / `_u2m` C-ABI setters. `cfg.Authenticator` is the single source of truth (last-writer-wins, matching Thrift). - **Unsupported auth is sentinel-wrapped** — token-provider / external / federated authenticators reject with `%w`-wrapped `ErrNotSupportedByKernel`, the same programmatic-fallback contract every other unsupported kernel option follows. **Session setup** - **Initial namespace** — post-connect `USE CATALOG` / `USE SCHEMA` with identifier quoting (the kernel C ABI has no namespace setter). - **Metric-view metadata** — `config.EffectiveSessionParams()` folds the server conf in backend-neutrally, so the kernel path sends the identical conf Thrift does. **Type rendering** - **INTERVAL** day-time & year-month rendering in `internal/arrowscan` (kernel returns native arrow values; formatted Go-side to the Thrift path's string form). Handles the `math.MinInt64` / `math.MinInt32` negation bound without overflow. - Parity coverage for **TIMESTAMP vs TIMESTAMP_NTZ** and **VARIANT / GEOMETRY** (live-probed on both backends). **Query execution & telemetry** - **Bound query parameters** via the kernel raw-param C ABI (`kernel_statement_bind_parameter`). The positional/named + SQL-NULL/empty-string decision is a pure `paramBindArg` seam, unit-tested under `CGO_ENABLED=0`. - **Kernel errors surfaced as `DBExecutionError`** carrying sqlstate + server query id, so `errors.As` → `SqlState()` / `QueryId()` works on the kernel path. The execute path deliberately reports **non-retryable** (a sent statement may have committed — no double-write), matching Thrift. - **Server query id** via a real `StatementID()` accessor (`kernel_executed_statement_query_id`), threaded into EXECUTE_STATEMENT telemetry, plus CLOSE_STATEMENT telemetry on the result-read path. - **Cancelled execute** evicts a session-fatal conn and preserves the kernel error metadata (multi-`%w`: `errors.Is(DeadlineExceeded)` AND `errors.As(*KernelError)` both hold). **Richer TLS (Go options)** - CA-bundle + independent hostname-skip setters mapped to the kernel's own TLS knobs (defensive PEM copy; the wholesale custom `WithTransport` stays rejected). **Kernel logging** - Kernel log output routed through the driver log level with connId/corrId/queryId correlation; allocation-free above Debug (guards the Arrow-batch hot path). - **scope note (please read before flagging):** - This unifies the *control knob*, not the *sink*, and that boundary is intentional. `DATABRICKS_LOG_LEVEL`/`SetLogLevel` now drives both the Go binding lines and the kernel's Rust lines from one level. But they stay two physical streams: Go lines go through `logger.Logger` (and honor `logger.SetLogOutput`), while the kernel's Rust lines go straight to stderr — the kernel C ABI exposes **no sink/callback hook**, so there is currently no way to route them through the Go logger. Consequences a reviewer will notice, all expected: - A custom `SetLogOutput`/file captures only the Go lines; Rust lines stay on stderr. - With the default sink both land on stderr but only *race* there — not a single ordered, merged stream. - The Rust subscriber's level is sampled once, at the first kernel connect. Fully merging both into one ordered sink requires a kernel log callback and is tracked separately (<TICKET>); it's out of scope here because it's a kernel-side ABI change, not a driver one. **CI** - A scheduled + dispatchable **nightly E2E** workflow that runs the credential-gated e2e + Thrift-vs-kernel parity suites against a real warehouse (the ordinary CI injects no warehouse secrets, so those tests otherwise skip). SEA-via-kernel integration leg gated behind a kernel label. ## Notable behavior - Bound query parameters are now supported (previously rejected at connect). - **Staging operations remain rejected** — the kernel path can't perform the local file transfer and the C ABI surfaces no staging signal. ## Testing - **Default pure-Go** (`CGO_ENABLED=0`): `go build` / `go vet` / `go test ./...` — 24 packages ok. - **Tagged kernel path** (`CGO_ENABLED=1 -tags databricks_kernel`, linked against a locally-built kernel `.a` at `KERNEL_REV`): 24 packages ok. - **Live e2e + parity** against a real warehouse: all pass — data types (incl. variant/geometry/decimal), NTZ, params-vs-Thrift (10/10), CloudFetch, cancellation, connection pool (40/40), StatementID, initial namespace, and **M2M** (OAuth client-credentials). - `gofmt` clean; Isaac Review clean (0 final comments). ## Merge gate - **Re-pin `KERNEL_REV` at merge time.** It currently points at the kernel dependency's PR-head SHA (GC-able once that kernel change merges); bump it to the resulting kernel `main` SHA, re-sync the cgo drift assertions in `cgo.go` if any signatures changed, and run `make test-kernel` against the new rev. This pull request and its description were written by Isaac. --------- Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Context for reviewers
Adds a scheduled CI workflow that runs the credential-gated E2E suites against a real test SQL warehouse, for both the Thrift backend and the SEA-via-kernel backend (the alternative backend that talks to Databricks SQL through the Rust
databricks-sql-kernelstatic lib over a C ABI, behind thedatabricks_kernelbuild tag).Stack position:
Reviewer note on the small test-file diffs: the changes to
kernel_e2e_test.go/kernel_parity_test.goare credential-variable renames (see §Credentials), not behavior changes. The substance of the PR is the newnightly-e2e.yml.What
A scheduled (07:00 UTC daily) + manually dispatchable workflow that runs the credential-gated end-to-end suites against a real test SQL warehouse. The ordinary "Go" workflow injects no warehouse secrets, so every E2E test
t.Skip()s there and real-warehouse behaviour (large multi-page CloudFetch, S3 downloads, drain-past-deadline, real auth) is otherwise not exercised in CI.Closes PECOBLR-3308.
Two jobs, one test warehouse
thrift-e2e— pure-Go (CGO_ENABLED=0); runs the high-value CloudFetch drain-past-caller-deadline + exact-row-count scenarios (-count=1, not-short, so the-short-guarded exact-row-count test runs).kernel-e2e— reuses thego.ymlkernel-build machinery (JFrog + cargo proxy, pinned Rust, kernel App-token clone, kernel-lib/cargo caches), then runs thedatabricks_kernel-tagged E2E funcs plus the Thrift-vs-kernel parity funcs (TestKernelThriftParity/TestKernelParamsVsThrift), which need both the kernel lib and live credentials.Both jobs are gated on
vars.NIGHTLY_E2E_ENABLED != 'false', so the workflow can be disabled via a repo variable without editing the file.Credentials (one secret set)
Both suites now read the same
DATABRICKS_PECOTESTING_*credentials, so a single secret set drives the whole workflow. Rather than map env-var names in the workflow, the kernel E2E / parity test helpers were renamed from the ad-hocDATABRICKS_HOST/_HTTP_PATH/_TOKENvars to the PECOTESTING set (with the_TOKEN_PERSONALfallback the Thrift suite already uses). Each job has a fail-loud credential guard so a missing secret errors instead of silently skipping into a misleading green — a silent skip reporting green is exactly the gap this closes.Live validation before merge (done — both jobs green, tests actually ran)
The workflow's only triggers are
schedule+workflow_dispatch, both gated to the default branch, so a feature branch can't exercise it normally. A temporarypush:trigger was added to validate against the real warehouse, then removed once validation passed (no temp trigger ships).The first validation run failed at
make kernel-libbecause the pinnedKERNEL_REVwas orphaned when the kernel dependency PR force-pushed (the documented GC merge-gate actually firing). The whole stack was re-pinned to a fetchable kernel rev, and the re-run went green:The 0-SKIP counts confirm the credentials wired through and the tests genuinely executed — the whole point of the ticket.
M2M kept skipped (decision)
TestKernelE2EM2Mis excluded (-skip) — not a silent skip. It's the only test that needs a service-principalDATABRICKS_CLIENT_ID/_CLIENT_SECRET, and we don't add an SP credential to the public repo for one test when (a) the PAT already covers the real-warehouse path for both backends and (b) the M2M setter wiring is already unit-covered byTestSetAuthByMode. It runs locally for a developer who exports the two vars; the job comment documents how to enable it in CI.Remaining step (repo admin — cannot be done from a PR)
Provision these repository secrets to make the nightly run green (the kernel job's App-token secrets already exist):
DATABRICKS_PECOTESTING_SERVER_HOSTNAMEDATABRICKS_PECOTESTING_HTTP_PATH2DATABRICKS_PECOTESTING_TOKEN(or_TOKEN_PERSONAL)Until then the credential-guard step fails by design (never a silent skip). A failed scheduled run surfaces via GitHub's built-in notification (no external webhook).
Testing
SKIPcleanly with no creds (the safe local/PR default); the renames caused no regressions — defaultCGO_ENABLED=0and taggeddatabricks_kernelsuites both 24 pkg ok / 0 fail.go test -list/-skipconfirmed the kernel job selects all E2E + both parity funcs and excludes M2M.Stacking
Stacked on #401 (
mani/sea-kernel-log-unify).This pull request and its description were written by Isaac.